Pip adversarial review — PR #113 (goggles#108)

Verdict: APPROVE — 0 blocking, 1 suggestion, 2 nitpicks.

The fix is correct, minimal, and exactly mirrors the prescribed remedy and the create_events() precedent: a single audit_file.groups.add(fallback_group) inside the transaction.atomic() block, guarded by fallback_group is not None, alongside the existing updated_at bump. Wholesale-quarantined uploads now appear in groups_for_audit_file (API response + detail page), audit_files_for_group (Files tab), and the group file_count. groups.add is idempotent, so the fallback_group is None path and the dedup races are unaffected. All 156 forensics tests pass. The regression test meaningfully exercises both helper paths and the API group/groups shape on the real non-UTF-8 byte path. No security/privacy regression — linking only attaches the file to the same fallback group its FK already pointed at, and nothing sensitive is logged.

🔴 BLOCKING

none

💡 SUGGESTION

• except Exception quarantine path is uncovered for the M2M link — forensics/ingest.py:323-325. save_invalid_upload has two callers: the non-UTF-8 decode path (ingest.py:159) and the defense-in-depth except Exception quarantine (ingest.py:251). The new test (tests.py:1097) exercises only the non-UTF-8 path. The existing except Exception test (test_unexpected_ingest_error_quarantines_upload_not_500, tests.py:2118) posts to the no-group endpoint, so fallback_group is None and the new groups.add branch is never hit there. Consider a quarantine-path upload to a group endpoint asserting the fallback-group link, since #108 names both as in-scope. (Same code line is shared, so this is coverage, not correctness.)

🔧 NITPICK

• Dedup early-returns still skip the M2M link — forensics/ingest.py:289-291 and 327-329. Re-uploading identical invalid bytes with a different fallback group than the original returns created=False and never adds the new group. This is pre-existing, matches the valid path's identical behavior (ingest.py:178-180), is keyed on file-content hash (arguably correct dedup semantics), and is out of scope for #108. Flagging only so it's an explicit, conscious non-goal. No change requested.
• groups_for_audit_file(...) == [fallback_group] assertion relies on pk-based __eq__ — forensics/tests.py:1117-1118. The annotated audit_files_for_group row equals the plain instance via Django pk comparison; works and is fine, just non-obvious.

Sensitive paths touched: none.

Adversarial review delegated to Cursor CLI (claude-opus-4-8-xhigh), findings verified independently against the checked-out branch. CI: Django tests pending; pip-audit pass; CodeRabbit rate-limited (no findings).